fix(a2a): use protocol-standard contextId as sessionId instead of req…#1827
fix(a2a): use protocol-standard contextId as sessionId instead of req…#1827pymBupt wants to merge 1 commit into
Conversation
…uiring metadata hardcoded key Previously, AgentScopeAgentExecutor.getSessionId() read sessionId exclusively from message.metadata["sessionId"], forcing all A2A clients to hardcode this custom metadata key — inappropriate for multi-client interoperability. The A2A protocol defines Message.contextId as the standard field for session/conversation context tracking. Changes: - Prefer contextId (via RequestContext.getContextId()) as the sessionId source - Fallback to metadata["sessionId"] for backward compatibility - Add 3 unit tests covering the resolution strategy Resolves agentscope-ai#1603 Related: agentscope-ai#820
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR changes AgentScopeAgentExecutor.getSessionId() to prefer the A2A protocol-standard Message.contextId field over the custom metadata["sessionId"] key, while maintaining backward compatibility via fallback. This correctly addresses issue #1603 where all A2A clients were forced to hardcode a custom metadata key. The change is minimal (~24 lines in production code), backward-compatible, and well-documented with Javadoc. Three new unit tests cover the priority resolution strategy. The direction is correct and the implementation is clean. Minor suggestion: add test coverage for the edge case where context.getContextId() returns null.
|
|
||
| @Test | ||
| @DisplayName("Should use contextId as sessionId when contextId is present") | ||
| void testSessionIdFromContextId() throws JSONRPCError { |
There was a problem hiding this comment.
[recommended] Test coverage gap: The existing 3 tests cover contextId present, metadata fallback, and both present. Consider adding a test for the edge case where context.getContextId() returns null (not empty string) and no metadata sessionId exists — verifying the method returns empty string. This path is handled correctly in the code (contextId != null check) but has no test.
|
|
||
| @Test | ||
| @DisplayName("Should use contextId as sessionId when contextId is present") | ||
| void testSessionIdFromContextId() throws JSONRPCError { |
There was a problem hiding this comment.
[nitpick] Test code duplication: The three test cases share extensive mock setup boilerplate (MessageSendParams mock, AtomicReference<AgentRequestOptions> capture, mockAgentRunner.stream stubbing, executor.execute call). Consider extracting a private helper method like executeAndCaptureOptions(contextId, metadataMap) to reduce duplication.
AgentScopeJavaBot
left a comment
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This PR changes AgentScopeAgentExecutor.getSessionId() to prefer the A2A protocol-standard Message.contextId field over the custom metadata["sessionId"] key, while maintaining backward compatibility via fallback. This correctly addresses issue #1603 where all A2A clients were forced to hardcode a custom metadata key. The change is minimal (~24 lines in production code), backward-compatible, and well-documented with Javadoc. Three new unit tests cover the priority resolution strategy. The direction is correct and the implementation is clean. Minor suggestion: add test coverage for the edge case where context.getContextId() returns null.
|
|
||
| @Test | ||
| @DisplayName("Should use contextId as sessionId when contextId is present") | ||
| void testSessionIdFromContextId() throws JSONRPCError { |
There was a problem hiding this comment.
[recommended] Test coverage gap: The existing 3 tests cover contextId present, metadata fallback, and both present. Consider adding a test for the edge case where context.getContextId() returns null (not empty string) and no metadata sessionId exists — verifying the method returns empty string. This path is handled correctly in the code (contextId != null check) but has no test.
|
|
||
| @Test | ||
| @DisplayName("Should use contextId as sessionId when contextId is present") | ||
| void testSessionIdFromContextId() throws JSONRPCError { |
There was a problem hiding this comment.
[nitpick] Test code duplication: The three test cases share extensive mock setup boilerplate (MessageSendParams mock, AtomicReference<AgentRequestOptions> capture, mockAgentRunner.stream stubbing, executor.execute call). Consider extracting a private helper method like executeAndCaptureOptions(contextId, metadataMap) to reduce duplication.
AgentScopeJavaBot
added
bug
3 participants
…uiring metadata hardcoded key
Previously, AgentScopeAgentExecutor.getSessionId() read sessionId exclusively from message.metadata["sessionId"], forcing all A2A clients to hardcode this custom metadata key — inappropriate for multi-client interoperability.
The A2A protocol defines Message.contextId as the standard field for session/conversation context tracking.
Changes:
Resolves #1603
Related: #820
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.12, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
[Please describe the background, purpose, changes made, and how to test this PR]
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)